Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(floodsub): Add option to configure the maximum message length #5588

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

romac
Copy link

@romac romac commented Sep 3, 2024

Description

This PR adds a max_message_len option to FloodsubConfig for configuring the maximum message length.

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@romac romac marked this pull request as draft September 3, 2024 12:43
@romac
Copy link
Author

romac commented Sep 3, 2024

Sorry, did not mean to open that PR already. Will leave as draft until properly tested on our side.

@dariusc93 dariusc93 changed the title feat(floodsub): Add max_message_len option to FloodsubConfig for configuring the maximum message length feat(floodsub): Add FloodsubConfig::max_message_len for configuring the maximum message length Sep 3, 2024
@dariusc93 dariusc93 changed the title feat(floodsub): Add FloodsubConfig::max_message_len for configuring the maximum message length feat(floodsub): Add option to configure the maximum message length Sep 3, 2024
Copy link
Member

@dariusc93 dariusc93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM so far. Could you bump the minor version in both Cargo.toml and update the changelog entry?

@romac romac marked this pull request as ready for review September 4, 2024 13:58
@romac
Copy link
Author

romac commented Sep 4, 2024

Ready for review. CI failure does not seem related to this PR.

@dariusc93
Copy link
Member

Ready for review. CI failure does not seem related to this PR.

Youre right. That is unrelated and would be handled in a different PR in the future.

@dariusc93 dariusc93 requested a review from jxs September 4, 2024 17:14
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks! Just one question

@@ -82,7 +82,7 @@ libp2p-connection-limits = { version = "0.4.0", path = "misc/connection-limits"
libp2p-core = { version = "0.42.0", path = "core" }
libp2p-dcutr = { version = "0.12.0", path = "protocols/dcutr" }
libp2p-dns = { version = "0.42.0", path = "transports/dns" }
libp2p-floodsub = { version = "0.45.0", path = "protocols/floodsub" }
libp2p-floodsub = { version = "0.46.0", path = "protocols/floodsub" }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem like a breaking change, do we need a minor version bump instead of patch?

Copy link
Member

@dariusc93 dariusc93 Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would assume that the introduction of a new public field in FloodsubConfig could be considered as one, especially given that it doesnt use the non_exhaustive attribute. I could be wrong though, but would have to check semver on that. If it is the case that it is a breaking change, we could set this PR to draft until the next round of breaking changes.

Copy link
Author

@romac romac Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I unfortunately do not see a way around doing a minor version bump, at least according to cargo-semver-checks:

argo semver-checks --baseline-version 0.45.0 --release-type patch
     Parsing libp2p-floodsub v0.46.0 (current)
      Parsed [   2.067s] (current)
     Parsing libp2p-floodsub v0.45.0 (baseline, cached)
      Parsed [   0.170s] (baseline)
    Checking libp2p-floodsub v0.45.0 -> v0.46.0 (assume patch change)
     Checked [   0.013s] 79 checks: 78 pass, 1 fail, 0 warn, 0 skip

--- failure constructible_struct_adds_field: externally-constructible struct adds field ---

Description:
A pub struct constructible with a struct literal has a new pub field. Existing struct literals must be updated to include the new field.
        ref: https://doc.rust-lang.org/reference/expressions/struct-expr.html
       impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.34.0/src/lints/constructible_struct_adds_field.ron

Failed in:
  field FloodsubProtocol.max_message_len in /Users/romac/Informal/Code/rust-libp2p/protocols/floodsub/src/protocol.rs:43
  field FloodsubConfig.max_message_len in /Users/romac/Informal/Code/rust-libp2p/protocols/floodsub/src/lib.rs:54
  field FloodsubRpc.max_message_len in /Users/romac/Informal/Code/rust-libp2p/protocols/floodsub/src/protocol.rs:155
  field FloodsubRpc.max_message_len in /Users/romac/Informal/Code/rust-libp2p/protocols/floodsub/src/protocol.rs:155

     Summary semver requires new major version: 1 major and 0 minor checks failed
    Finished [   2.267s] libp2p-floodsub

Note that what cargo-semver-checks calls a major version corresponds to a minor version bump for libp2p-floodsub since the crate is at 0.x.y.

Copy link
Author

@romac romac Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, if we are doing a breaking change in libp2p-floodsub, would you accept a PR to align the naming of types with the other crates, as per #2217?

ie. rename FloodsubConfig to just Config, Floodsub to Behavior and FloodsubEvent to Event, etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, if we are doing a breaking change in libp2p-floodsub, would you accept a PR to align the naming of types with the other crates, as per #2217?

ie. rename FloodsubConfig to just Config, Floodsub to Behavior and FloodsubEvent to Event, etc.

We can do this in a separate PR and just use an alias pointing to the new names with the alias being deprecated :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, will open a PR for this sometime soon then :)

Copy link
Member

@jxs jxs Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's the problem, those fields don't need to be public in FloodsubConfig, and probably FloodsubRpc and FloodsubProtocol also don't need to be public. Could you also do it in the subsequent PR @romac?
Thanks!

Copy link
Contributor

mergify bot commented Oct 25, 2024

This pull request has merge conflicts. Could you please resolve them @romac? 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants